Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[FLINK-11662] Disable task to fail on checkpoint errors #8745

Merged
merged 4 commits into from
Jun 27, 2019
Merged

[FLINK-11662] Disable task to fail on checkpoint errors #8745

merged 4 commits into from
Jun 27, 2019

Conversation

Myasuka
Copy link
Member

@Myasuka Myasuka commented Jun 14, 2019 •
edited
Loading

What is the purpose of the change

FLINK-11662 has been a known and urgent issue after Flink-1.8 release. There exist two parts to fix this problem, one is from task side to disable task to fail on checkpoint errors. Another part is to let JM to decide when to fail the whole job which has been part of work in #8322.

This PR mainly focus on the task side and choose to base on #8322 before it merged since that is very unlikely to still have significant changes at this point. Once #8322 merged, this PR would then rebased on the latest code.

Brief change log

  • Deprecate isFailTaskOnCheckpointError and setFailTaskOnCheckpointError in ExecutionConfig.
  • Deprecate isFailOnCheckpointingErrors and setFailOnCheckpointingErrors in CheckpointConfig.
  • Remove FailingCheckpointExceptionHandler

Verifying this change

This change added tests and can be verified as follows:

  • Refactor CheckpointExceptionHandlerConfigurationTest and CheckpointExceptionHandlerTest to verify the default logic changed.
  • Refactor StreamTaskTest to verify the default logic changed on task side.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): yes
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? JavaDocs

@Myasuka Myasuka changed the title Task not fail checkpoint [FLINK-11662] Task not fail checkpoint Jun 14, 2019
@Myasuka Myasuka changed the title [FLINK-11662] Task not fail checkpoint [FLINK-11662] Disable task to fail on checkpoint errors Jun 14, 2019
@flinkbot
Copy link
Collaborator

flinkbot commented Jun 14, 2019 •
edited
Loading

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Review Progress

  • ❓ 1. The [description] looks good.
  • ❓ 2. There is [consensus] that the contribution should go into to Flink.
  • ❗ 3. Needs [attention] from.
  • ❓ 4. The change fits into the overall [architecture].
  • ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.


The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

@Myasuka
Copy link
Member Author

Myasuka commented Jun 14, 2019

@flinkbot attention @StefanRRichter

StefanRRichter
StefanRRichter requested changes Jun 19, 2019
View reviewed changes
Copy link
Contributor

@StefanRRichter StefanRRichter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For most parts, changes look good to me, with some smaller comments. However, in the current form the PR can break existing setups because it ignores the user's with from the deprecated flag. I would suggest to i) have the deprecated flag influence the number of tolerable checkpoint errors (0 or unlimited) and ii) have the new flag always overrule the old flag if used together and maybe iii) log a warning if they are used together.

flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java Outdated Show resolved Hide resolved
flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/tasks/StreamTask.java Outdated Show resolved Hide resolved
@Myasuka Myasuka force-pushed the task-not-fail-checkpoint branch 3 times, most recently from a95242e to 2bdd142 Compare June 23, 2019 07:49
@Myasuka Myasuka force-pushed the task-not-fail-checkpoint branch from 0e55b99 to 3d8c77d Compare June 23, 2019 17:45
StefanRRichter
StefanRRichter approved these changes Jun 27, 2019
View reviewed changes
Copy link
Contributor

@StefanRRichter StefanRRichter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the good work @Myasuka ! Changes look good, merging.

@StefanRRichter StefanRRichter merged commit b760d55 into apache:master Jun 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanRRichter StefanRRichter StefanRRichter approved these changes

Assignees
No one assigned
Labels
component=Runtime/Checkpointing review=description?
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Myasuka @flinkbot @StefanRRichter @rmetzger